Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create a generic AVR target: avr-unknown-unknown #131651

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Create a generic AVR target: avr-unknown-unknown #131651

wants to merge 1 commit into from
+136 −59

Conversation

Patryk27
Copy link
Contributor

This commit removes the avr-unknown-gnu-atmega328 target and replaces it with a more generic avr-unknown-unknown variant that can be specialized using -C target-cpu (e.g. -C target-cpu=atmega328p).

I've decided to go with the -unknown tag (i.e. avr-unknown-unknown instead of avr-unknown-gnu), because that's the name LLVM already uses - I see no reason to diverge here.

Seizing the day, I'm adding myself as the maintainer of this target - I've been already fixing the bugs anyway, might as well make it official 🙂

Related discussion: #131171.

@rustbot
Copy link
Collaborator

rustbot commented Oct 13, 2024

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 13, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 13, 2024

Some changes occurred in src/doc/rustc/src/platform-support

cc @Noratrieb

Some changes occurred in src/tools/compiletest

cc @jieyouxu

These commits modify compiler targets.
(See the Target Tier Policy.)

@Patryk27
Copy link
Contributor Author

cc @workingjubilee?

@Patryk27 Patryk27 mentioned this pull request Oct 13, 2024
Open
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Patryk27
Copy link
Contributor Author

Ouch, I'm not sure why the test is failing - rustc --print=all-target-specs-json -Z unstable-options does return avr-unknown-unknown.

@workingjubilee
Copy link
Member

my first instinct is "bootstrap problem?"

@workingjubilee workingjubilee added the O-AVR Target: AVR processors (ATtiny, ATmega, etc.) label Oct 13, 2024
@jieyouxu
Copy link
Member

I'll take a look

@jieyouxu jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2024
jieyouxu
jieyouxu approved these changes Oct 13, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test and bootstrap side of things LGTM modulo the compiletest header test thing. Didn't look at the compiler side.

@Patryk27 Patryk27 mentioned this pull request Oct 14, 2024
Open
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jfrimmel jfrimmel mentioned this pull request Oct 16, 2024
Merged
@rustbot rustbot added the A-compiletest Area: The compiletest test runner label Oct 19, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@tgross35
Copy link
Contributor

tgross35 commented Oct 19, 2024 •
edited
Loading

Just checking since I don't see it linked, did this get an MCP? That is usually the approval step for changing targets.

(if not, issue template here https://github.com/rust-lang/compiler-team/issues)

@Patryk27
Copy link
Contributor Author

Patryk27 commented Oct 19, 2024 •
edited
Loading

I haven't created MCP, wasn't sure on the process (especially considering AVR is a rather unpopular target).

If you think this needs a MCP, I can create one.

@tgross35
Copy link
Contributor

I think an MCP makes sense just to make everyone aware of the change, it's the usual process even for tier 3 targets. I don't see it actually being a problem, this makes sense to me.

Overall I think PR looks pretty good, left a few small comments. Thanks for the thorough build and test instructions, that is awesome to have.

@Patryk27
Copy link
Contributor Author

Okie, I'll create an MCP then 🙂

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Patryk27
Create a generic AVR target: avr-unknown-unknown
f8643b8 
This commit removes the `avr-unknown-gnu-atmega328` target and replaces
it with a more generic `avr-unknown-unknown` variant that can be
specialized using `-C target-cpu` (e.g. `-C target-cpu=atmega328p`).

I've decided to go with the `-unknown` tag (i.e. `avr-unknown-unknown`
instead of `avr-unknown-gnu`), because that's the name LLVM already uses
- I see no reason to diverge here.

Seizing the day, I'm adding myself as the maintainer of this target -
I've been already fixing the bugs anyway, might as well make it
official.

Related discussion:
rust-lang#131171
@Patryk27 Patryk27 mentioned this pull request Oct 20, 2024
Open
@Patryk27
Copy link
Contributor Author

^ MCP created: rust-lang/compiler-team#800.

@Patryk27
Copy link
Contributor Author

Patryk27 commented Nov 1, 2024

cc @tgross35? 👀

@madsmtm
Copy link
Contributor

madsmtm commented Nov 2, 2024 •
edited
Loading

So, Rust's naming scheme isn't really consistent, but I think the target should be named avr-unknown-none (since it has target_os = "none"), and then if you end up wanting variations, they should probably be avr-unknown-none-*.

There is also precedent for omitting the unknown part, examples include wasm32v1-none, msp430-none-elf and riscv32imc-esp-espidf. So that's also an option (calling it avr-none, and future variants avr-none-*).

Otherwise highly in favour!

Also, could you edit the main description to also note that you're removing the old target (just to make it clear)?

madsmtm
madsmtm approved these changes Nov 2, 2024
View reviewed changes
src/doc/rustc/src/platform-support/avr-unknown-unknown.md

## Target maintainers

-[Patryk Wychowaniec](https://github.com/Patryk27) <pwychowaniec@pm.me>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
-[Patryk Wychowaniec](https://github.com/Patryk27) <pwychowaniec@pm.me>
- [Patryk Wychowaniec](https://github.com/Patryk27) <pwychowaniec@pm.me>

src/doc/rustc/src/platform-support/avr-unknown-unknown.md
Comment on lines +13 to +14
This target is only cross-compiled; x86\_64 Linux, x86\_64 MacOS and aarch64
MacOS hosts are confirmed to work, but in principle any machine able to run
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
This target is only cross-compiled; x86\_64 Linux, x86\_64 MacOS and aarch64
MacOS hosts are confirmed to work, but in principle any machine able to run
This target is only cross-compiled; x86\_64 Linux, x86\_64 macOS and Aarch64
macOS hosts are confirmed to work, but in principle any machine able to run

@tgross35
Copy link
Contributor

tgross35 commented Nov 2, 2024

cc @tgross35? 👀

Somebody needs to second the MCP. The changes make sense to me but I don't know enough about it to make a decision here - I'll poke on Zulip.

@bors
Copy link
Contributor

bors commented Nov 2, 2024

☔ The latest upstream changes (presumably #132497) made this pull request unmergeable. Please resolve the merge conflicts.

@jfrimmel jfrimmel mentioned this pull request Nov 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgross35 tgross35 tgross35 left review comments

@jieyouxu jieyouxu jieyouxu approved these changes

@madsmtm madsmtm madsmtm approved these changes

Assignees

@compiler-errors compiler-errors

Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc O-AVR Target: AVR processors (ATtiny, ATmega, etc.) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Patryk27 @rustbot @rust-log-analyzer @workingjubilee @jieyouxu @tgross35 @madsmtm @bors @compiler-errors